-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bug(Stock Value report crashes server with too much data) #4556
bug(Stock Value report crashes server with too much data) #4556
Conversation
@jniles @mbayopanda I managed to optimize the report, please do the review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to test this, but it still crashes for me. My virtual machine only has 4GB of RAM, but that should be more than enough. After 2 minutes, the request times out and retries. It also pegs my MySQL CPU usage at over 300%.
I'm not sure this solves the problem at all. I still think, as mentioned in our previous discussion, that something like period_total
is the way to go. That way, we are doing very little computation in the actual report itself.
migration-hev_mai_2020.sql
Outdated
SET character_set_database = 'utf8mb4'; | ||
SET collation_database = 'utf8mb4_unicode_ci'; | ||
SET CHARACTER SET utf8mb4, CHARACTER_SET_CONNECTION = utf8mb4; | ||
SET collation_connection = 'utf8mb4_unicode_ci'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really necessary? The SQL dump should be exporting with these defaults. Is that not the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really necessary? The SQL dump should be exporting with these defaults. Is that not the case?
This portion of code was added when I rebase with master, I haven't added it personally
* but also the cumulative value of entries *quantityCum* and exits *cumOut* | ||
* and the quantity in stock after each movement of stock *quantityStock* | ||
*/ | ||
const stockValues = await db.exec(sqlGetInventories, [db.bid(options.depot_uuid), options.dateTo]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sp, if I am understanding this correctly, this code will generate 1 + N
SQL queries in a transaction, where N
is the number of inventory items in a depot. At Vanga, the total number of inventory items is currently 784. At IMCK, the total number is 1302. So we could potentially get hundreds of SQL queries launched from a single query here.
... I'm not sure how optimal this is in low-end hardware like a Raspberry Pi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the way I understood the logic of the stock value report came down to retrieving the last line of the stock card report, my idea was to launch several requests on the stock movement table in order to calculate for each unit cost weight and the stock value after each transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had given up on the idea of using an aggregation table because of the risk of having backdated stock movements
}); | ||
}); | ||
|
||
let stockTolal = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a typo - it should be "stockTotal"
<tr> | ||
<td style="width:50%">{{inventory_name}}</td> | ||
<td class="text-right">{{stockQtt}}</td> | ||
<td class="text-right">{{currency (multiply stockUnitCost ../rate) ../currency_id}}</td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do this multiplication before putting it in the template?
</tr> | ||
{{/each}} | ||
<!-- no data --> | ||
{{#if emptyResult}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need for this "emptyResult" check. Just use {{#each}}{{else}}{{/each}}
as discussed in WhatsApp.
So, it would look like this:
{{#each stockValues}}
{{! ... etc ... }}
{{else}}
<tr><th class="text-center" colspan="4">{{translate 'STOCK.NO_DATA'}}</th></tr>
{{/each}}
const options = (typeof (_options.params) === 'string') ? JSON.parse(_options.params) : _options.params; | ||
data.dateTo = options.dateTo; | ||
data.depot = await db.one('SELECT * FROM depot WHERE uuid=?', [db.bid(options.depot_uuid)]); | ||
const stockValues = await db.exec('CALL stockValue(?,?,?);', [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not longer using the stockValue()
procedure, let's remove it from the stored procedures.
|
||
const stockValueElements = options.exclude_zero_value | ||
? stockValues[0].filter(item => item.stockValue > 0) : stockValues[0]; | ||
? stockValues.filter(item => item.stockValue > 0) : stockValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway we can apply this filter higher in logic? Preferably in the SQL. It would probably improve performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be difficult seeing that the products we see the stock shortage of a product after on the last line, after having noted all the entries and exits from Stock
With me it works really well |
That's because you have a machine with 16GB of RAM that cost many, many dollars. We can't expect poor hospitals to buy these kinds of computers. |
I think the problem stems from the way in which stock managers calculate the stock value of a product, I fear that even if we only had computers with large capacities, even after 10 or 15 years. use, that the stock value report then crashes out again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Improvement and optimization of the stock value by deposit report closes Third-Culture-Software#4116
- Improvement and optimization of the stock value report, reduction of operations on the MYSQL side
b1d7df6
to
299cb15
Compare
@jniles i need a review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much better for performance. The query takes 10 seconds on my machine (22 seconds total time). It also doesn't peg MySQL at 300% CPU usage. See below.
I still think we can do better via period_total
style table, but this fixes the bug in production to an acceptable level where the report is usable. Great work!
bors r+
Build succeeded:
|
closes #4116